Skip to content

Make mctp-req generic #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make mctp-req generic #6

wants to merge 2 commits into from

Conversation

leiyu-bytedance
Copy link

No description provided.

@jk-ozlabs jk-ozlabs self-requested a review October 31, 2022 00:55
jk-ozlabs
jk-ozlabs requested changes Oct 31, 2022
View reviewed changes
Copy link
Member

@jk-ozlabs jk-ozlabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good: a couple of fixups mentioned in the code though.

I think we should simplify the user interface for this a little - I'd suggest making the data argument mandatory (sending sequential bytes was handy while bringing up the kernel interface, but now that's done, it's no longer very useful). That means we could drop the len argument too (since we can calculate that from the data argument). I'm also fine with making the type argument mandatory too - assuming PLDM is a bit strange (again, this was from the kernel bringup, where we just needed an arbitrary type value).

Then, we would no longer need the conditional code in the mctp_req() function that re-uses the rxbuf, and just always use separate buffers for tx & rx.

@leiyu-bytedance
Copy link
Author

Added a new commit to install the systemd units.

@jk-ozlabs
Copy link
Member

Added a new commit to install the systemd units.

That would be better as a separate PR.

@jk-ozlabs
Copy link
Member

@leiyu-bytedance just wanted to check-in on this - anything pending from your side?

@leiyu-bytedance
Copy link
Author

Oh, we totally missed this.
Will update the PRs a bit later

@leiyu-bytedance
mctp-req: Add data argument in usage
4324538 
The usage description is missing the `data` argument, add it and an
example of how it is used.

Signed-off-by: Lei YU <[email protected]>
@leiyu-bytedance
mctp-req: Make it generic
93f45c9 
The mctp-req was acting like a echo client that sends and receive the
same data.

Change the code to make it a generic sender and receiver, so that user
could use it to send all types of data and prints the received data.
* Add a `type` argument to specify the mctp type, default to 1 (PLDM).
* Remove the code that expects the same len and data for sent and
  received data.
* Add print of the received data.

Tested: Get UUID of an endpoint:

    $ mctp-req eid 9 type 0 data 80:03
    req:  sending to (net 1, eid 9), type 0, len 2
    req:  message from (net 1, eid 9) type 0 len 19
    data:
    0x00 0x03 0x00 0x7e 0x92 0x05 0xfc 0x01 0xc2 0xeb 0x11 0x80 0x00 0xb8
    0xce 0xf6 0xae 0xcd 0x16

Signed-off-by: Lei YU <[email protected]>
@leiyu-bytedance
Copy link
Author

Added a new commit to install the systemd units.

That would be better as a separate PR.

#13

@khangng-ampere
Copy link

@leiyu-bytedance The new branch lost some requested change above, you may want to review them again.

amboar added a commit to amboar/mctp that referenced this pull request Apr 15, 2025
@amboar
mctpd: add_peer: Update userdata pointer for active event sources
291ca86 
Avoids a heap UAF caught by ASAN:

    ==179005==ERROR: AddressSanitizer: heap-use-after-free on address 0x61f000000c38 at pc 0x55dfaa7fa308 bp 0x7ffe10264420 sp 0x7ffe10264418
    READ of size 8 at 0x61f000000c38 thread T0
        #0 0x55dfaa7fa307 in peer_endpoint_recover ../src/mctpd.c:2570
        CodeConstruct#1 0x7f9a43dadae3  (/lib/x86_64-linux-gnu/libsystemd.so.0+0x78ae3)
        CodeConstruct#2 0x7f9a43dade04 in sd_event_dispatch (/lib/x86_64-linux-gnu/libsystemd.so.0+0x78e04)
        CodeConstruct#3 0x7f9a43daf2e7 in sd_event_run (/lib/x86_64-linux-gnu/libsystemd.so.0+0x7a2e7)
        CodeConstruct#4 0x7f9a43daf506 in sd_event_loop (/lib/x86_64-linux-gnu/libsystemd.so.0+0x7a506)
        CodeConstruct#5 0x55dfaa80a609 in main ../src/mctpd.c:4547
        CodeConstruct#6 0x7f9a42c46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        CodeConstruct#7 0x7f9a42c46304 in __libc_start_main_impl ../csu/libc-start.c:360
        CodeConstruct#8 0x55dfaa7e38d0 in _start (mctp/build/test-mctpd+0x688d0)

    0x61f000000c38 is located 3000 bytes inside of 3040-byte region [0x61f000000080,0x61f000000c60)
    freed by thread T0 here:
        #0 0x7f9a436b78d5 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:85
        CodeConstruct#1 0x55dfaa7ef028 in add_peer ../src/mctpd.c:1419
        CodeConstruct#2 0x55dfaa7f1587 in endpoint_assign_eid ../src/mctpd.c:1601
        CodeConstruct#3 0x55dfaa7f55a0 in method_setup_endpoint ../src/mctpd.c:2038
        CodeConstruct#4 0x7f9a43d650ad  (/lib/x86_64-linux-gnu/libsystemd.so.0+0x300ad)

    previously allocated by thread T0 here:
        #0 0x7f9a436b78d5 in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:85
        CodeConstruct#1 0x55dfaa7ef028 in add_peer ../src/mctpd.c:1419
        CodeConstruct#2 0x55dfaa805741 in add_local_eid ../src/mctpd.c:4052
        CodeConstruct#3 0x55dfaa80627f in add_interface_local ../src/mctpd.c:4114
        CodeConstruct#4 0x55dfaa806ffa in setup_nets ../src/mctpd.c:4200
        CodeConstruct#5 0x55dfaa80a380 in main ../src/mctpd.c:4525
        CodeConstruct#6 0x7f9a42c46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

    SUMMARY: AddressSanitizer: heap-use-after-free ../src/mctpd.c:2570 in peer_endpoint_recover
    Shadow bytes around the buggy address:
      0x0c3e7fff8130: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
      0x0c3e7fff8140: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
      0x0c3e7fff8150: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
      0x0c3e7fff8160: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
      0x0c3e7fff8170: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    =>0x0c3e7fff8180: fd fd fd fd fd fd fd[fd]fd fd fd fd fa fa fa fa
      0x0c3e7fff8190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c3e7fff81a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c3e7fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c3e7fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c3e7fff81d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    Shadow byte legend (one shadow byte represents 8 application bytes):
      Addressable:           00
      Partially addressable: 01 02 03 04 05 06 07
      Heap left redzone:       fa
      Freed heap region:       fd
      Stack left redzone:      f1
      Stack mid redzone:       f2
      Stack right redzone:     f3
      Stack after return:      f5
      Stack use after scope:   f8
      Global redzone:          f9
      Global init order:       f6
      Poisoned by user:        f7
      Container overflow:      fc
      Array cookie:            ac
      Intra object redzone:    bb
      ASan internal:           fe
      Left alloca redzone:     ca
      Right alloca redzone:    cb
    ==179005==ABORTING

Signed-off-by: Andrew Jeffery <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jk-ozlabs jk-ozlabs jk-ozlabs requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leiyu-bytedance @jk-ozlabs @khangng-ampere